Convert custom_associations to array of symbols
#356
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shioyama
force-pushed
the
shioyama/convert_custom_associations_to_symbols
branch
from
5935590 to
8ef194d
Compare
shioyama
force-pushed
the
shioyama/convert_custom_associations_to_symbols
branch
from
8ef194d to
066bd48
Compare
shioyama
commented
May 18, 2023
| @@ -75,7 +75,7 @@ def initialize(configs = {}, config_path: nil) | |||
| root = config_path ? File.dirname(config_path) : "." | |||
| @root_path = T.let(File.expand_path(root), String) | |||
| @package_paths = T.let(configs["package_paths"] || "**/", T.any(String, T::Array[String])) | |||
| @custom_associations = T.let(configs["custom_associations"] || [], T::Array[Symbol]) | |||
| @custom_associations = T.let((configs["custom_associations"] || []).map(&:to_sym), T::Array[Symbol]) | |||
There was a problem hiding this comment.
I'm confused why Sorbet wasn't blowing up on this since the array wasn't an array of symbols 🤔
There was a problem hiding this comment.
Ah configs is a hash with untyped values.
davidstosik
approved these changes
May 18, 2023
There was a problem hiding this comment.
I too am surprised this has not been a problem already.
Looks like the packwerk.yml template would have given wrong instructions?
packwerk/lib/packwerk/generators/templates/packwerk.yml.erb
Lines 15 to 17 in 95caea0
Fix looks good to me! 👍🏻
| @@ -42,7 +42,7 @@ class ConfigurationTest < Minitest::Test | |||
| assert_equal ["{exclude_dir,bin,tmp}/**/*"], configuration.exclude | |||
| assert_equal app_dir, configuration.root_path | |||
| assert_equal "**/*/", configuration.package_paths | |||
| assert_equal ["custom_association"], configuration.custom_associations | |||
There was a problem hiding this comment.
yet if you set an array of strings for custom_associations then Packwerk will simply ignore them without any error.
So this test was "right", but testing an invalid scenario?
rafaelfranca
approved these changes
May 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What are you trying to accomplish?
Having symbols in a yaml array is less natural than an array of strings, yet if you set an array of strings for
custom_associationsthen Packwerk will simply ignore them without any error.And actually, the example config itself has an array of strings:
packwerk/lib/packwerk/generators/templates/packwerk.yml.erb
Lines 15 to 17 in 066bd48
What approach did you choose and why?
Just convert them to symbols when assigning from yaml.
What should reviewers focus on?
Why wasn't Sorbet blowing up on this? There's a
T.letassigning to an array of symbols, yet it was assigning an array of Strings, that should be a problem. 🤔Type of Change
Additional Release Notes
Include any notes here to include in the release description. For example, if you selected "breaking change" above, leave notes on how users can transition to this version.
If no additional notes are necessary, delete this section or leave it unchanged.
Checklist